-
Notifications
You must be signed in to change notification settings - Fork 0
プラン設定機能追加 #26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
プラン設定機能追加 #26
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds plan configuration functionality to the billing system, introducing endpoints for managing tenant pricing plans and tax rates. The changes enable users to view available pricing plans, tax rates, and current tenant plan information, as well as update tenant plans with reservation capabilities.
- Added four new REST endpoints for plan management operations
- Introduced
UpdateTenantPlanRequestmodel for plan update requests - Implemented plan reservation functionality using the SaaS SDK's
PlanReservationmodel
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
billing_router.py
Outdated
| tax_rate_id: str = None | ||
| using_next_plan_from: int = None |
Copilot
AI
Oct 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Optional[str] and Optional[int] instead of assigning None as default values. Import Optional from typing module for better type safety and clarity.
billing_router.py
Outdated
| except Exception as e: | ||
| raise HTTPException(status_code=500, detail="Internal server error") |
Copilot
AI
Oct 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message 'Internal server error' is too generic and doesn't provide useful debugging information. Consider logging the actual exception and providing a more specific error message.
billing_router.py
Outdated
| except Exception as e: | ||
| raise HTTPException(status_code=500, detail="Internal server error") |
Copilot
AI
Oct 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message 'Internal server error' is too generic and doesn't provide useful debugging information. Consider logging the actual exception and providing a more specific error message.
billing_router.py
Outdated
| ) | ||
|
|
||
| # 税率IDが指定されている場合のみ設定 | ||
| if request.tax_rate_id and request.tax_rate_id != "": |
Copilot
AI
Oct 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition request.tax_rate_id and request.tax_rate_id != \"\" can be simplified to just request.tax_rate_id since an empty string is falsy in Python, or use request.tax_rate_id.strip() if you want to handle whitespace-only strings.
| if request.tax_rate_id and request.tax_rate_id != "": | |
| if request.tax_rate_id: |
billing_router.py
Outdated
| plan_reservation.next_plan_tax_rate_id = request.tax_rate_id | ||
|
|
||
| # using_next_plan_fromが指定されている場合のみ設定 | ||
| if request.using_next_plan_from and request.using_next_plan_from > 0: |
Copilot
AI
Oct 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition checks both truthiness and > 0, but since using_next_plan_from is an int, checking > 0 alone would be sufficient and clearer (0 is falsy anyway).
| if request.using_next_plan_from and request.using_next_plan_from > 0: | |
| if request.using_next_plan_from > 0: |
| except Exception as e: | ||
| raise HTTPException(status_code=500, detail="Failed to update tenant plan") |
Copilot
AI
Oct 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message 'Failed to update tenant plan' is generic and doesn't provide useful debugging information. Consider logging the actual exception and providing more specific error details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 8 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
billing_router.py
Outdated
| if not auth_user.tenants: | ||
| raise HTTPException(status_code=400, detail="No tenants found for the user") |
Copilot
AI
Oct 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tenant check in get_pricing_plans is unnecessary since pricing plans are not tenant-specific. This endpoint retrieves all available pricing plans in the system, not tenant-specific data. The check should be removed as it incorrectly prevents users without tenants from viewing available plans.
| if not auth_user.tenants: | |
| raise HTTPException(status_code=400, detail="No tenants found for the user") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
対応しました。
billing_router.py
Outdated
| if not auth_user.tenants: | ||
| raise HTTPException(status_code=400, detail="No tenants found for the user") | ||
|
|
Copilot
AI
Oct 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tenant check in get_tax_rates is unnecessary since tax rates are not tenant-specific. This endpoint retrieves all available tax rates in the system, not tenant-specific data. The check should be removed as it incorrectly prevents users without tenants from viewing available tax rates.
| if not auth_user.tenants: | |
| raise HTTPException(status_code=400, detail="No tenants found for the user") | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
対応しました。
| tax_rates = TaxRateApi(api_client=pricing_api_client).get_tax_rates() | ||
| return tax_rates.tax_rates | ||
| except Exception as e: | ||
| raise HTTPException(status_code=500, detail="Failed to retrieve tax rates") |
Copilot
AI
Oct 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message lacks context about the actual error. Consider including the exception details or a more specific error message to aid in debugging. For example: detail=f\"Failed to retrieve tax rates: {str(e)}\"
| raise HTTPException(status_code=500, detail="Failed to retrieve tax rates") | |
| raise HTTPException(status_code=500, detail=f"Failed to retrieve tax rates: {str(e)}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
エラー詳細はprintでコンソール出力、フロントには出さない形式で対応。
|
|
||
| return response | ||
| except Exception as e: | ||
| raise HTTPException(status_code=500, detail="Failed to retrieve tenant detail") |
Copilot
AI
Oct 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message lacks context about the actual error. Consider including the exception details or a more specific error message to aid in debugging. For example: detail=f\"Failed to retrieve tenant detail: {str(e)}\"
| raise HTTPException(status_code=500, detail="Failed to retrieve tenant detail") | |
| raise HTTPException(status_code=500, detail=f"Failed to retrieve tenant detail: {str(e)}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
エラー詳細はprintでコンソール出力、フロントには出さない形式で対応。
|
|
||
| return {"message": "Tenant plan updated successfully"} | ||
| except Exception as e: | ||
| raise HTTPException(status_code=500, detail="Failed to update tenant plan") |
Copilot
AI
Oct 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message lacks context about the actual error. Consider including the exception details or a more specific error message to aid in debugging. For example: detail=f\"Failed to update tenant plan: {str(e)}\"
| raise HTTPException(status_code=500, detail="Failed to update tenant plan") | |
| raise HTTPException(status_code=500, detail=f"Failed to update tenant plan: {str(e)}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
エラー詳細はprintでコンソール出力、フロントには出さない形式で対応。
billing_router.py
Outdated
| if not tenant_id: | ||
| raise HTTPException(status_code=400, detail="tenant_id is required") |
Copilot
AI
Oct 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This validation is redundant since tenant_id is a required path parameter in the route definition. FastAPI automatically ensures path parameters are present and non-empty, so this check will never trigger and should be removed.
| if not tenant_id: | |
| raise HTTPException(status_code=400, detail="tenant_id is required") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
対応しました。
billing_router.py
Outdated
| if not tenant_id: | ||
| raise HTTPException(status_code=400, detail="tenant_id is required") |
Copilot
AI
Oct 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This validation is redundant since tenant_id is a required path parameter in the route definition. FastAPI automatically ensures path parameters are present and non-empty, so this check will never trigger and should be removed.
| if not tenant_id: | |
| raise HTTPException(status_code=400, detail="tenant_id is required") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
対応しました。
| # 税率IDが指定されている場合のみ設定 | ||
| if request.tax_rate_id: |
Copilot
AI
Oct 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition checks for truthiness (if request.tax_rate_id:) which will be False for empty strings, but the comment on line 476 indicates empty strings should be handled. Change to if request.tax_rate_id is not None: to match the behavior of next_plan_id and allow empty strings to be set.
| # 税率IDが指定されている場合のみ設定 | |
| if request.tax_rate_id: | |
| # 税率IDがNoneでない場合は設定(空文字も含む) | |
| if request.tax_rate_id is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
next_plan_idは解除予約で空文字が必要、tax_rate_idは不要のため対応しない。
No description provided.